Add LibGit2 CredentialPayload struct #23189

omus · 2017-08-09T11:09:08Z

Part of #20725.

Created the LibGit2.CredentialPayload structure which allows us to keep track of state between multiple credential callbacks. Currently, the payload allows us to:

Parse the url_ptr once
Simplify authenticate_ssh and authenticate_userpass signatures

A future PR will extend this to remove callback state information from all AbstractCredentials including:

Removing the authentication failure protection counter (Remove authentication failure protection counter #23575)
Removing the prompt_if_incorrect setting (Add settings to CredentialPayload #23690)
Removing the usesshagent setting (Move SSH agent state out of SSHCredentials #23641)

omus · 2017-08-09T13:24:29Z

base/libgit2/callbacks.jl

+                credid = "ssh://$(p.host)"
+                creds = get_creds!(unsafe_get(p.cache), credid, creds)
+            end
+            p.credential = Nullable(creds)


Previously, if user supplied a UserPasswordCredentials type and SSH was one of the allowed types we would just skip SSH authentication. The PR code changes this to attempt to first attempt to do an SSH authentication then do a user/password authentication. In the process the passed in user credentials will be overwritten and not used.

We may want to revise the logic further to better support when authentication can use either SSH or user/password but I'd like to avoid trying doing that at the moment as I don't have any real-world examples of this behaviour.

That seems unintuitive, wouldn't UserPasswordCredentials only be passed if the user expected them to be used?

Are any of the methods here visible externally, would this need deprecations anywhere?

That seems unintuitive, wouldn't UserPasswordCredentials only be passed if the user expected them to be used?

Yes, the user should be expecting them to be used but it is possible they cannot be used. Let me go over a quick example. If you run:

LibGit2.clone("ssh://git@github.com/user/my-private-repo.git", payload=Nullable(LibGit2.UserPasswordCredentials("foo", "bar")))

The credential callback will be requesting making a request for SSH credentials. But since the user passed in UserPasswordCredentials we can't do anything with these and we need to ignore these credentials (PR behaviour) or abend (original behaviour).

In a completely hypothetical scenario where the allowed_types mask stated that either a SSH credential or user/pass credential could be returned (I have yet to see this) and we ran the same code above we would end up prompting the user for SSH credentials even though we could just try using the passed in UserPassCredentials. This is the scenario I was referring to in my earlier comment.

Are any of the methods here visible externally, would this need deprecations anywhere?

I don't think any deprecations are required as the change now falls back to prompting the user for credentials instead of abending when the explicit credential fails.

Is there still a way to force error-on-bad-credentials behavior? prompting could be a bad idea in some non-interactive uses

I have a PR in the pipeline which will allow the option of disallowing prompting for all credentials. For now however I'll try to restore the original abending behaviour for this PR.

omus · 2017-08-10T14:46:48Z

I'm currently testing the existing behaviour and noticed something strange. If you provide an explicit user/pass credential and the callback expects an SSH credential you get a warning about the credential being incompatible. However, if you supply a SSH credential and the callback expects a user/pass credential the callback will end up ignoring the explicit credential.

I'll make sure to make this behaviour consistent.

omus · 2017-08-12T05:13:59Z

For now however I'll try to restore the original abending behaviour for this PR.

I've revised the PR to restore the original abort behaviour when the explicit credentials fail. In order to make sure I've kept the original behaviour I wrote several new tests to capture the existing behaviour with explicit credentials and the credential cache.

I'm currently testing the existing behaviour and noticed something strange. If you provide an explicit user/pass credential and the callback expects an SSH credential you get a warning about the credential being incompatible. However, if you supply a SSH credential and the callback expects a user/pass credential the callback will end up ignoring the explicit credential.

I'll make sure to make this behaviour consistent.

Explicit credentials which don't match the requested authentication method now always error the same way.

tkelman · 2017-08-12T05:24:16Z

base/libgit2/types.jl

@@ -183,12 +185,16 @@ Matches the [`git_remote_callbacks`](https://libgit2.github.com/libgit2/#HEAD/ty
    payload::Ptr{Void}
 end

-function RemoteCallbacks(credentials::Ptr{Void}, payload::Ref{Nullable{AbstractCredentials}})
-    RemoteCallbacks(credentials=credentials, payload=pointer_from_objref(payload))
+function RemoteCallbacks(credentials_cb::Ptr{Void}, payload::Ref{Payload})


should this be Ref{<:Payload} ?

Sure. I can also change the constructor below to use Ref(...) instead of Ref{Payload}(...)

omus · 2017-08-15T03:47:52Z

Any additional comments? I think I'll merge this later tomorrow.

LibGit2 credential callback now always uses a CredentialPayload struct which allows us keep callback state separate from credentials. Implementation at this point is minimal to allow us to more thoroughly test the existing behaviour.

The two code paths for handling the the SSH and username/password `allowed_types` differ enough that some bugs have cropped up. These changes make the two code paths nearly identical which should remove any inconsistences.

omus · 2017-08-15T20:54:07Z

Rebased just to test against Circle CI. Will merge after the CI finishes

omus · 2017-08-16T01:07:13Z

So much for all my green check marks. Prior to rebasing only Circle CI was failing due to the lack of configuration file. The latest has a unrelated failures on AppVeyor and the Travis CI on Mac had a failure with spawn. Going ahead with merge.

StefanKarpinski · 2017-08-16T03:05:41Z

Since @travis-ci has been such a disaster lately, we should consider it ok as long as Circle CI or Travis passes, not both. We should probably see about getting Mac testing on Circle CI.

tkelman · 2017-08-16T03:26:28Z

most of the mac failures we hit are julia bugs and would happen anywhere

tkelman · 2017-08-16T04:27:00Z

circle also doesn't currently support multi os matrix builds the way travis does, as I understand it. they may add support for that at some future time.

omus added the libgit2 The libgit2 library or the LibGit2 stdlib module label Aug 9, 2017

omus commented Aug 9, 2017

View reviewed changes

omus force-pushed the cv/credential-payload branch from f5e0fcf to b6b6f64 Compare August 12, 2017 05:09

tkelman reviewed Aug 12, 2017

View reviewed changes

omus added 5 commits August 15, 2017 15:53

Add minimal CredentialPayload implementation

5ef2ca9

LibGit2 credential callback now always uses a CredentialPayload struct which allows us keep callback state separate from credentials. Implementation at this point is minimal to allow us to more thoroughly test the existing behaviour.

Correct SSH explicit test

cebf09a

Error on incompatible explicit credential

ecd4285

Store parsed callback URL in payload

3d4ff9d

Make credential handling more consistent

5cd220c

The two code paths for handling the the SSH and username/password `allowed_types` differ enough that some bugs have cropped up. These changes make the two code paths nearly identical which should remove any inconsistences.

omus force-pushed the cv/credential-payload branch from 29cea17 to 5cd220c Compare August 15, 2017 20:53

omus merged commit be74e58 into master Aug 16, 2017

omus deleted the cv/credential-payload branch August 16, 2017 01:07

omus mentioned this pull request Aug 17, 2017

Remove outdated LibGit2.get_creds! methods #23286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LibGit2 CredentialPayload struct #23189

Add LibGit2 CredentialPayload struct #23189

omus commented Aug 9, 2017 •

edited

Loading

omus Aug 9, 2017 •

edited

Loading

tkelman Aug 9, 2017

omus Aug 9, 2017 •

edited

Loading

tkelman Aug 9, 2017

omus Aug 9, 2017 •

edited

Loading

omus commented Aug 10, 2017

omus commented Aug 12, 2017

tkelman Aug 12, 2017

omus Aug 12, 2017

omus commented Aug 15, 2017

omus commented Aug 15, 2017

omus commented Aug 16, 2017

StefanKarpinski commented Aug 16, 2017

tkelman commented Aug 16, 2017

tkelman commented Aug 16, 2017

Add LibGit2 CredentialPayload struct #23189

Add LibGit2 CredentialPayload struct #23189

Conversation

omus commented Aug 9, 2017 • edited Loading

omus Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

tkelman Aug 9, 2017

Choose a reason for hiding this comment

omus Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

tkelman Aug 9, 2017

Choose a reason for hiding this comment

omus Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

omus commented Aug 10, 2017

omus commented Aug 12, 2017

tkelman Aug 12, 2017

Choose a reason for hiding this comment

omus Aug 12, 2017

Choose a reason for hiding this comment

omus commented Aug 15, 2017

omus commented Aug 15, 2017

omus commented Aug 16, 2017

StefanKarpinski commented Aug 16, 2017

tkelman commented Aug 16, 2017

tkelman commented Aug 16, 2017

omus commented Aug 9, 2017 •

edited

Loading

omus Aug 9, 2017 •

edited

Loading

omus Aug 9, 2017 •

edited

Loading

omus Aug 9, 2017 •

edited

Loading